-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make build tool independent #7
make build tool independent #7
Conversation
This PR goes hand in hand with prebuild/node-gyp-build#4. |
Very cool. Did you test that this works on windows? |
not yet, will do |
I don't see anything that wouldnt work but you never know ... windows |
I tested this with taglib2 and it works :) |
@juliangruber So in order to use this, you would land this patch and run https://github.com/juliangruber/prebuildify-load ? |
@bcomnes That is outdated by now. We can instead add the missing functionality to |
Sounds good |
Maybe a package rename is in order, but we can table that for now. PR(s) welcome! |
Don't quite have a vision of what you mean, maybe if we write it up? |
To clarify, I thought it was okay to add functionality to the existing modules and that we could later choose to rename But on second thought, there's a fundamental difference between "scripts": {
"install": "node-gyp-build"
} "scripts": {
"install": "prebuildify-load || node-gyp build"
} The latter approach avoids needing integration code specific to e.g. If we can agree (@mafintosh @ralphtheninja) that this is the preferred approach, we could move |
|
@mafintosh My point was, with the approach of |
I don’t personally have a need or a valid perspective on this so I’ll abstain |
Old issue, but I think this makes sense actually. @vweevers should we revisit this? |
It's a good feature and doesn't add complexity. I'm for it, as long as it's semver-major because it will break |
we could revive mine as well, I'm open to hand anything over |
@@ -103,17 +103,18 @@ function run (cmd, cwd, env, cb) { | |||
|
|||
function build (target, runtime, opts, cb) { | |||
var args = [ | |||
'rebuild', | |||
'--target=' + target | |||
'install', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we consider npm run install
instead of npm install
? The latter will slow builds down (doing unnecessary work), especially with prebuildify-cross
'--target=' + target | ||
'install', | ||
'--target=' + target, | ||
'--abi=' + abi.getAbi(target, runtime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need to add --build-from-source
, if the install
script is node-gyp-build
.
@juliangruber Could you register your fork on npm so we can use it? This fixes #49 |
You can also publish it :) Or use the npm github syntax |
no need to be limited to node-gyp :) I came up with this because a project I want to use this on uses cmake-js.
I tested this in https://github.com/voltraco/node-taglib2 so far, but it should work everywhere as node-gyp reads all its config also from the environment variables npm sets.